Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix "using Windows PHP in Cygwin"-check in shim script #155

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Fix "using Windows PHP in Cygwin"-check in shim script #155

merged 1 commit into from
Nov 21, 2023

Conversation

HoldYourWaffle
Copy link
Contributor

Fixes #153.

This check was introduced in ae29413, but as far as I can tell "Cygwin PHP" understands Windows-style paths just fine, so the extra "Windows PHP"-check could be removed.

Supersedes #154, apparently GitHub trips if you rename a branch...

@johnstevenson
Copy link
Member

Thanks, but this breaks Cygwin PHP. Did you test it?

@HoldYourWaffle
Copy link
Contributor Author

I did, like I mentioned as far as I can tell "Cygwin PHP" understands Windows-style paths just fine.

However, I just noticed that Using Cygwin - Using native Win32 paths advises against relying on this behavior, so I'll see if I can find an even better way to do this :)

@HoldYourWaffle HoldYourWaffle marked this pull request as draft November 20, 2023 17:04
@HoldYourWaffle HoldYourWaffle force-pushed the fix--windows-php-check branch 2 times, most recently from 5d7be15 to fbd50b8 Compare November 20, 2023 20:25
@HoldYourWaffle HoldYourWaffle marked this pull request as ready for review November 20, 2023 20:26
@HoldYourWaffle
Copy link
Contributor Author

HoldYourWaffle commented Nov 20, 2023 •
edited
Loading

@johnstevenson Found a better way!

If the Windows-translated root path (C:\cygwin64) is a prefix of the Windows-translated PHP path (C:\tools\php82\php for Windows, C:\cygwin64\usr\bin\php for Cygwin), then we're using a Cygwin binary and need to translate the path.

It feels a teeny tiny bit janky, but it seems to be impossible to consistently resolve mounted paths to their cygdrive equivalent.

@HoldYourWaffle HoldYourWaffle changed the title Remove "using Windows PHP"-check from shim script Fix "using Windows PHP in Cygwin"-check in shim script Nov 20, 2023
johnstevenson
johnstevenson reviewed Nov 21, 2023
View reviewed changes
Copy link
Member

@johnstevenson johnstevenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks like a good way of doing it

src/shims/composer Outdated Show resolved Hide resolved
src/shims/composer Outdated Show resolved Hide resolved
src/shims/composer Outdated Show resolved Hide resolved
@HoldYourWaffle
Copy link
Contributor Author

@johnstevenson Fixed!
I remember starting out with -m and it not working, but it seems to work now, very strange...

@johnstevenson johnstevenson merged commit 4c2ccc5 into composer:main Nov 21, 2023
4 checks passed
@johnstevenson
Copy link
Member

Thanks for this.

@HoldYourWaffle HoldYourWaffle deleted the fix--windows-php-check branch November 21, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnstevenson johnstevenson johnstevenson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Shim script wrongly assumes all "Windows paths" in Cygwin start with /proc/cygdrive
2 participants
@HoldYourWaffle @johnstevenson